Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix URL encoded filenames #104

Closed
wants to merge 7 commits into from
Closed

Fix URL encoded filenames #104

wants to merge 7 commits into from

Conversation

mofeing
Copy link
Contributor

@mofeing mofeing commented Dec 22, 2021

I was trying to use doc2dash with the documentation of quimb but the screen show the following error.

Converting intersphinx docs from "html" to "./html.docset".
Parsing documentation...
Added 2,494 index entries.
Adding table of contents meta data...  [##################################################-------------------------------------------]   53%  00:00:18
Traceback (most recent call last):
  File "/Users/mofeing/Develop/quimb/.venv/bin/doc2dash", line 8, in <module>
    sys.exit(main())
  File "/Users/mofeing/Develop/quimb/.venv/lib/python3.8/site-packages/click/core.py", line 829, in __call__
    return self.main(*args, **kwargs)
  File "/Users/mofeing/Develop/quimb/.venv/lib/python3.8/site-packages/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/Users/mofeing/Develop/quimb/.venv/lib/python3.8/site-packages/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/mofeing/Develop/quimb/.venv/lib/python3.8/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/Users/mofeing/Develop/quimb/.venv/lib/python3.8/site-packages/doc2dash/__main__.py", line 227, in main
    toc.close()
  File "/Users/mofeing/Develop/quimb/.venv/lib/python3.8/site-packages/doc2dash/parsers/utils.py", line 132, in patch_anchors
    patch_files(pbar)
  File "/Users/mofeing/Develop/quimb/.venv/lib/python3.8/site-packages/doc2dash/parsers/utils.py", line 112, in patch_files
    with codecs.open(full_path, mode="r", encoding="utf-8") as fp:
  File "/Users/mofeing/.pyenv/versions/3.8.2/lib/python3.8/codecs.py", line 905, in open
    file = builtins.open(filename, mode, buffering)
FileNotFoundError: [Errno 2] No such file or directory: './html.docset/Contents/Resources/Documents/calculating%20quantities.html'

Problem was that the repo generates some HTML files with spaces in their names and other files referencing to them encodes their names in URL form. Adding a URL decoding step solves the problem and does not interfere with other filenames.

@hynek
Copy link
Owner

hynek commented Dec 23, 2021

Thanks for fixing the decode; would you mind adding a changelog entry and test?

@mofeing
Copy link
Contributor Author

mofeing commented Dec 23, 2021

Sure.

About the changelog, where should I write it? I guess that inside CHANGELOG.rst but should I create a write it inside 2.4.0 or create a new 2.4.1 section?

@hynek
Copy link
Owner

hynek commented Dec 23, 2021

Oh yeah, looks like I've forgotten to create a new heading! Please do!

@mofeing
Copy link
Contributor Author

mofeing commented Dec 23, 2021

Done!

CHANGELOG.rst Outdated Show resolved Hide resolved
tests/parsers/test_utils.py Outdated Show resolved Hide resolved
def entries_url_format():
return [
ParserEntry(name="foo", type="Method", path="foo%20bar.html#foo"),
ParserEntry(name="qux", type="Class", path="foo%20bar.html"),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's a point to making this a fixture? I think it can be safely inlined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can just add a new entry in the entries fixture but I wanted to have a separate test just for checking URL-encoded filenames. I see know it can be done without a new fixture. Give me a sec.

toc.send(e)
toc.close()
assert [
TOCEntry(name="foo", type="Method", anchor="foo")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, doesn't this mean that you've lost a TOCEntry somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm lost on writing a test. I though I could just add a new entry to the entries fixture, copy the test_single_entry test in test_utils.py and change it so it checks all the entries have been correctly parsed. But I don't understand what is happenning in here.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, I’ll have a look after Xmas. Happy holidays and thanks for your work so far!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, and happy holidays to you too!

tests/parsers/test_utils.py Show resolved Hide resolved
Co-authored-by: Hynek Schlawack <hs@ox.cx>
@mofeing
Copy link
Contributor Author

mofeing commented Jan 18, 2022

@hynek any new on this? in jcmgray/quimb#108 we are waiting on this PR to be merged.
i was a lost on writing a test, but count on me if there is some way i can help.

@hynek
Copy link
Owner

hynek commented Jan 21, 2022

I couldn't push it against your branch so I've fixed and merged it by hand in d8b770b – thanks!

@hynek hynek closed this Jan 21, 2022
@hynek
Copy link
Owner

hynek commented Jan 21, 2022

(looks like github didn't recognize your git identity so I've force pushed (😱) 158d7dc)

@mofeing
Copy link
Contributor Author

mofeing commented Jan 21, 2022

about my git identity i don't know what happened 😨
anyway, thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants